Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding ReasonForError usage: #384

Merged
merged 2 commits into from
Apr 6, 2019
Merged

Adding ReasonForError usage: #384

merged 2 commits into from
Apr 6, 2019

Conversation

acornies
Copy link
Contributor

@acornies acornies commented Mar 8, 2019 •
edited

To address #362 by mapping more appropriate http error responses for secrets API.

Description

  • Required update k8s.io/apimachinery to 1.9
  • Added processErrorReasons mapping private function

Motivation and Context

  • I have raised an issue to propose this change (required)

How Has This Been Tested?

TODO

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@acornies
Adding ReasonForError usage:
52e8743 
- Required update k8s.io/apimachinery to 1.9
- Added processErrorReasons mapping private function

Signed-off-by: Andrew Cornies <acornies@gmail.com>
@alexellis
Copy link
Member

Is CI blocked? Do we know why?

@LucasRoesler
Copy link
Member

I don't see any link for CI details, which is really weird

@alexellis
Copy link
Member

Why is this PR draft? Is that why? Is this WIP or ready for review @acornies ?

alexellis
alexellis reviewed Mar 10, 2019
View reviewed changes
handlers/secrets.go
case k8serrors.ReasonForError(err) == metav1.StatusReasonBadRequest:
log.Printf("Secret %s error reason: %s, %v\n", context, metav1.StatusReasonBadRequest, err)
return http.StatusBadRequest
case k8serrors.ReasonForError(err) == metav1.StatusReasonForbidden:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my concern with this approach is that we don't give the reason for the error back to the caller.

We should also be able to test this new method that has been broken out with unit tests when we have a final design on how it is going to work.

I am not super keen on logging in the function. I'd rather see you return the Status Code plus a message that we then chose to log in the calling function - thereby applying Single Responsibility Principle (SRP)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@LucasRoesler
Copy link
Member

LucasRoesler commented Mar 10, 2019 •
edited

The last build on travis looks like this https://travis-ci.org/openfaas/faas-cli/builds/504025019

for 54ee31dd7e3d538783ba9466f81dc5ab98a793bd which does not match the current commit

@acornies
Copy link
Contributor Author

Yeah, the draft PRs mess with CI usually. I chose it so that we could deliberate on the change first (pre-unit tests as well). I'll update with recent feedback.

@acornies
Updates based on PR feedback:
a4f434f 
- move logging back to caller
- return both http status code and k8s reason
- add tests for default and conflict reasons

Signed-off-by: Andrew Cornies <acornies@gmail.com>
@acornies
Copy link
Contributor Author

I've updated the PR. Can someone flip this PR status to non-draft mode? I don't have permissions.

@alexellis
Copy link
Member

I would have thought since it's your PR you can set it out of draft. What do the GH docs say? This is a new feature and I don't entirely see the point of it

@acornies
Copy link
Contributor Author

Normally you can: https://help.github.com/en/articles/changing-the-stage-of-a-pull-request. Is it because I'm not a member of any teams?

@acornies acornies marked this pull request as ready for review March 12, 2019 12:57
@derek derek bot added the no-dco label Mar 12, 2019
@derek
Copy link

derek bot commented Mar 12, 2019

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --sign-off and then git push --force.

@derek derek bot removed the no-dco label Mar 12, 2019
@alexellis alexellis merged commit 41f1640 into openfaas:master Apr 6, 2019
@acornies acornies deleted the error_reasons branch June 7, 2019 01:52
@LucasRoesler LucasRoesler mentioned this pull request Jul 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexellis alexellis alexellis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@acornies @alexellis @LucasRoesler